Enable REST protocol between Aggregator and Collaborator by ishaileshpant · Pull Request #1500 · securefederatedai/openfederatedlearning

ishaileshpant · 2025-03-27T04:27:56Z

Summary

Enable rest protocol support for TaskRunner API

Type of Change (Mandatory)

Feature enhancement

Description (Mandatory)

This PR adds REST protocol support for TaskRunner API there by allowing running federation with REST protocol for seamless integration with managed/public API gateways

Unit Test report

Full Unit test report

ishaileshpant · 2025-05-13T09:17:24Z

@psfoley / @teoparvanov is it fine if this refactor / task 'Retry mechanism config pullout, remove the hardcoding' be taken in a subsequent PR ?

payalcha · 2025-05-13T10:50:14Z

@ishaileshpant
https://github.com/securefederatedai/openfl/pull/1612/files#diff-13d501ef02760f7d8b8de7d82adc709762a4d2bb56aef8cdbbf8ddf06b4daafbR54
Please uncomment the test once PR get merged. So that basic no-op connectivity will get tested with your PR.

kminhta · 2025-05-14T21:16:41Z

Thanks @ishaileshpant ! This is looking good. Btw, can you manually run https://github.com/ishaileshpant/openfl/actions/workflows/task_runner_flower_e2e.yml on your branch? I don't anticipate any major issues since it'll just run gRPC, but it'll be good to catch any unforeseen issues early

ishaileshpant · 2025-05-15T04:45:07Z

Thanks @ishaileshpant ! This is looking good. Btw, can you manually run https://github.com/ishaileshpant/openfl/actions/workflows/task_runner_flower_e2e.yml on your branch? I don't anticipate any major issues since it'll just run gRPC, but it'll be good to catch any unforeseen issues early

Thanks for the inputs @kta-intel - have just validated and the default workflow works https://github.com/ishaileshpant/openfl/actions/runs/15036703510/job/42259707667 better to validate this with protocol set to 'rest' - wdyt?

kminhta · 2025-05-18T14:23:18Z

Thanks for the inputs @kta-intel - have just validated and the default workflow works https://github.com/ishaileshpant/openfl/actions/runs/15036703510/job/42259707667 better to validate this with protocol set to 'rest' - wdyt?

thanks for checking, and yes I agree it would be good to validate with rest

ishaileshpant · 2025-05-19T04:04:13Z

Thanks for the inputs @kta-intel - have just validated and the default workflow works https://github.com/ishaileshpant/openfl/actions/runs/15036703510/job/42259707667 better to validate this with protocol set to 'rest' - wdyt?

thanks for checking, and yes I agree it would be good to validate with rest

Validated with 'rest' protocol (w/o client auth) https://github.com/ishaileshpant/openfl/actions/runs/15099298504/job/42438127648

payalcha · 2025-05-19T15:57:51Z

RestAPI is not working as expected - Collaborator is not able to connect with Aggregator

[15:54:02] [plan.py:240] INFO Building `openfl.pipelines.NoCompressionPipeline` Module.
[15:54:02] [aggregator_client.py:130] WARNING Initializing Aggregator REST Client (EXPERIMENTAL API - Not for production use)
[15:54:02] [plan.py:240] INFO Building `openfl.component.Collaborator` Module.
[15:54:02] [collaborator.py:130] WARNING Argument `device_assignment_policy` is deprecated and will be removed in the future.
[15:54:02] [aggregator_client.py:295] ERROR Certificate unknown error - this usually means the server's certificate is not trusted
[15:54:02] [aggregator_client.py:299] ERROR Please verify that:
[15:54:02] [aggregator_client.py:300] ERROR 1. The root certificate contains all necessary intermediate certificates
[15:54:02] [aggregator_client.py:301] ERROR 2. The server's certificate is properly signed by a trusted CA
[15:54:02] [aggregator_client.py:302] ERROR 3. The hostname matches the certificate's subject
[15:54:02] [aggregator_client.py:332] ERROR Connection error: HTTPSConnectionPool(host='localhost', port=49467): Max retries exceeded with url: /experimental/v1/tasks?collaborator_id=collaborator1&federation_uuid=plan.yaml_1cb6b3e5 (Caused by SSLError(SSLError(1, '[SSL: SSLV3_ALERT_CERTIFICATE_UNKNOWN] sslv3 alert certificate unknown (_ssl.c:2578)')))
[15:54:02] [aggregator_client.py:334] ERROR Connection error details: HTTPSConnectionPool(host='localhost', port=49467): Max retries exceeded with url: /experimental/v1/tasks?collaborator_id=collaborator1&federation_uuid=plan.yaml_1cb6b3e5 (Caused by SSLError(SSLError(1, '[SSL: SSLV3_ALERT_CERTIFICATE_UNKNOWN] sslv3 alert certificate unknown (_ssl.c:2578)')))

ishaileshpant · 2025-05-20T13:02:29Z

RestAPI is not working as expected - Collaborator is not able to connect with Aggregator

[15:54:02] [plan.py:240] INFO Building `openfl.pipelines.NoCompressionPipeline` Module.
[15:54:02] [aggregator_client.py:130] WARNING Initializing Aggregator REST Client (EXPERIMENTAL API - Not for production use)
[15:54:02] [plan.py:240] INFO Building `openfl.component.Collaborator` Module.
[15:54:02] [collaborator.py:130] WARNING Argument `device_assignment_policy` is deprecated and will be removed in the future.
[15:54:02] [aggregator_client.py:295] ERROR Certificate unknown error - this usually means the server's certificate is not trusted
[15:54:02] [aggregator_client.py:299] ERROR Please verify that:
[15:54:02] [aggregator_client.py:300] ERROR 1. The root certificate contains all necessary intermediate certificates
[15:54:02] [aggregator_client.py:301] ERROR 2. The server's certificate is properly signed by a trusted CA
[15:54:02] [aggregator_client.py:302] ERROR 3. The hostname matches the certificate's subject
[15:54:02] [aggregator_client.py:332] ERROR Connection error: HTTPSConnectionPool(host='localhost', port=49467): Max retries exceeded with url: /experimental/v1/tasks?collaborator_id=collaborator1&federation_uuid=plan.yaml_1cb6b3e5 (Caused by SSLError(SSLError(1, '[SSL: SSLV3_ALERT_CERTIFICATE_UNKNOWN] sslv3 alert certificate unknown (_ssl.c:2578)')))
[15:54:02] [aggregator_client.py:334] ERROR Connection error details: HTTPSConnectionPool(host='localhost', port=49467): Max retries exceeded with url: /experimental/v1/tasks?collaborator_id=collaborator1&federation_uuid=plan.yaml_1cb6b3e5 (Caused by SSLError(SSLError(1, '[SSL: SSLV3_ALERT_CERTIFICATE_UNKNOWN] sslv3 alert certificate unknown (_ssl.c:2578)')))

This has been fixed in latest push - the above error comes only in mTLS mode - TLS w/o client auth worked

Now with the fix all the cases should be fine

psfoley

This is looking good, @ishaileshpant. I've tested this locally with mTLS and I think this is in pretty good shape overall. I have a few minor comments from my review. Two other things I would request before merge:

A REST E2E test for CI. This is a big enough feature I want to be confident this is in a continuous working state.
Basic documentation. There should either be a minor page that explains how to toggle between gRPC / REST in the plan, or a note of this in the real world federation page (related to proxies that don't support gRPC).

After this is merged for OpenFL 1.9 I see an opportunity for:

More function / constant reuse between gRPC and REST implementations. I called out the possibility of more consistency between the gRPC / REST datastream messages, but other areas we could streamline are disallowed TLS versions. This feels like it should be defined once somewhere else and imported.
Additional tests for TLS / mTLS plan configurations. You are pretty well covered for functional tests for mTLS, but it would be good to test around just TLS as well (no client auth)

psfoley · 2025-05-21T04:17:06Z

+        # Serialize the TaskResults first
+        task_results_bytes = task_results.SerializeToString()
+        logger.debug(f"TaskResults serialized size: {len(task_results_bytes)} bytes")
+
+        # Create a DataStream message containing the TaskResults bytes
+        data_stream = base_pb2.DataStream(size=len(task_results_bytes), npbytes=task_results_bytes)
+
+        # Create an empty DataStream to signal end of stream
+        end_stream = base_pb2.DataStream(size=0, npbytes=b"")
+
+        # Serialize both messages
+        data_bytes = data_stream.SerializeToString()
+        end_bytes = end_stream.SerializeToString()
+
+        # Create length-prefixed stream format
+        stream_data = (
+            struct.pack(">I", len(data_bytes))  # Length prefix for first message
+            + data_bytes  # First message
+            + struct.pack(">I", len(end_bytes))  # Length prefix for second message
+            + end_bytes  # Second message (empty message signals end)
+        )


Is it necessary to encode the data stream this way? If so that's fine. I'm asking because it would be nice to have more symmetry with the gRPC implementation.

The current implementation is more like a batched RPC call with a custom binary protocol format rather than true streaming and i kept it this way to support both HTTP/1.1 and enhance for HTTP/2 true streaming later on - if you agree then post this iteration we should look at chunked transfer to better support both http/1.1 and eventually http/2 by enhancing when needs arises - wdyt?

ishaileshpant · 2025-05-21T06:51:25Z

A REST E2E test for CI. This is a big enough feature I want to be confident this is in a continuous working state.

We already have this in place as @payalcha has pointed out - we can uncomment this post the PR is merged

ishaileshpant · 2025-05-21T06:52:38Z

After this is merged for OpenFL 1.9 I see an opportunity for:

Totally agree, already have some ideas on doing this post the PR merge

- add new AggregatorClientInterface to allow switching b/w grpc and rest - endhance existing AggregatorGRPCClient to start using AggregatorClientInterface - added new transport package for rest with AggregatorRESTClient implementing AggregatorClientInterface - added streaming api support with custom content-type - added various connection flag for streaming request - send additional header key "Sender" for better request logging at server side - aligned Rest and gRPC client for most of the init params - added AggregatorRESTServer and necesary changes in aggregator cli and federated/plan get_server method - added transport_protocol settings in defaults/network.yaml, defaulted the same to 'grpc' - reduced cyclomatic complexity of Rest Server - fixed protobuf streaming issue for v1/task/results API - added more detailed logging for task progression and metadata for each api calls - pinned Flask version to latest stable 3.1.0 - addressing review comments - 13th-May - added ping api and `collaborato` constructor hint for `AggregatorClientInterface` - added send_message_to_server in client and AggregatorClientInterface, Rest Server is already at parity - changed base uri for REST server to 'experimental/v1', adjusted the client and tests accordingly - fixed issue related to mTLS in REST server/client - disabled TLS 1.2 in both server/client rebased 21st.May.1 Signed-off-by: Shailesh Pant <shailesh.pant@intel.com>

ishaileshpant force-pushed the native_rest_api branch 6 times, most recently from 541fe87 to af43e36 Compare April 2, 2025 19:39

ishaileshpant force-pushed the native_rest_api branch from af43e36 to 0c13600 Compare April 14, 2025 10:39

ishaileshpant marked this pull request as ready for review April 15, 2025 08:48

ishaileshpant requested review from MasterSkepticista, kminhta, psfoley and teoparvanov as code owners April 15, 2025 08:48

ishaileshpant force-pushed the native_rest_api branch 4 times, most recently from cc563d9 to ff9e063 Compare April 17, 2025 20:05

ishaileshpant force-pushed the native_rest_api branch from ff9e063 to 22c4219 Compare April 22, 2025 08:52

ishaileshpant requested review from noopurintel, payalcha, rahulga1, rajithkrishnegowda, tanwarsh and theakshaypant as code owners April 22, 2025 08:52

ishaileshpant force-pushed the native_rest_api branch 4 times, most recently from 6683e0d to e615e1d Compare April 24, 2025 08:59

ishaileshpant changed the title ~~[WIP - do not review] Enable REST protocol between Aggregator and Collaborator~~ Enable REST protocol between Aggregator and Collaborator Apr 24, 2025

ishaileshpant force-pushed the native_rest_api branch 2 times, most recently from d77306c to 0beb7fd Compare April 25, 2025 09:54

ishaileshpant force-pushed the native_rest_api branch from 1b64503 to 66627a6 Compare May 13, 2025 09:13

ishaileshpant force-pushed the native_rest_api branch 2 times, most recently from 77ba225 to bde8f0c Compare May 13, 2025 10:39

ishaileshpant force-pushed the native_rest_api branch 5 times, most recently from 83aad3c to 74f2340 Compare May 14, 2025 11:42

ishaileshpant force-pushed the native_rest_api branch from 74f2340 to 098abe5 Compare May 15, 2025 08:54

kminhta mentioned this pull request May 18, 2025

Batched fetching of tensors to reduce RPC calls [1/2] #1575

Open

ishaileshpant force-pushed the native_rest_api branch from 098abe5 to 5dd5300 Compare May 18, 2025 18:04

ishaileshpant force-pushed the native_rest_api branch from 5dd5300 to 6a742d2 Compare May 19, 2025 05:23

kminhta approved these changes May 19, 2025

View reviewed changes

ishaileshpant force-pushed the native_rest_api branch from 9c316b7 to 9377caa Compare May 20, 2025 12:56

ishaileshpant force-pushed the native_rest_api branch from 59327cf to 04abc9e Compare May 20, 2025 17:27

theakshaypant approved these changes May 21, 2025

View reviewed changes

psfoley requested changes May 21, 2025

View reviewed changes

ishaileshpant force-pushed the native_rest_api branch from 04abc9e to c3f3844 Compare May 21, 2025 07:52

teoparvanov merged commit 7fcecd3 into securefederatedai:develop May 21, 2025
37 checks passed

Conversation

ishaileshpant commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Type of Change (Mandatory)

Description (Mandatory)

Unit Test report

Full Unit test report

Uh oh!

ishaileshpant commented May 13, 2025

Uh oh!

payalcha commented May 13, 2025

Uh oh!

kminhta commented May 14, 2025

Uh oh!

ishaileshpant commented May 15, 2025

Uh oh!

kminhta commented May 18, 2025

Uh oh!

ishaileshpant commented May 19, 2025

Uh oh!

payalcha commented May 19, 2025

Uh oh!

ishaileshpant commented May 20, 2025

Uh oh!

psfoley left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

psfoley May 21, 2025

Choose a reason for hiding this comment

Uh oh!

ishaileshpant May 21, 2025

Choose a reason for hiding this comment

Uh oh!

ishaileshpant commented May 21, 2025

Uh oh!

ishaileshpant commented May 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ishaileshpant commented Mar 27, 2025 •

edited

Loading

psfoley left a comment •

edited

Loading